Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(apig-v1-adapter): lowercase headers #259

Merged
merged 1 commit into from
Sep 6, 2024
Merged

fix(apig-v1-adapter): lowercase headers #259

merged 1 commit into from
Sep 6, 2024

Conversation

tcandens
Copy link

@tcandens tcandens commented Sep 4, 2024 •
edited
Loading

Description of change

Adding back the default behavior of emulating Node.js http by lowercasing all request headers. Large parts of JS ecosystem assume that these headers, particularly cookie header, can be accessed with the lowercase key.

This behavior was present in past versions of the ApiGatewayV1Adapter so please correct me if there is good reason that this was changed, but from my perspective it appears it may have just been missed. This past PR is an example, #25.

Also, potentially related to #73

Pull-Request Checklist

  • Code is up-to-date with the main branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Added documentation inside www/docs/main folder.
  • Included new files inside index.doc.ts.
  • The new commits follow conventions outlined in the conventional commit spec

@H4ad
Copy link
Owner

H4ad commented Sep 5, 2024

Did you had any issue with the headers not being flattered?

I changed to not lowercase because the headers are always lowercase when came from API Gateway V1, so for performance reasons, I just skipped the lowercase.

@tcandens
Copy link
Author

tcandens commented Sep 5, 2024 •
edited
Loading

Thank you for looking!

Did you had any issue with the headers not being flattered?

We haven't had any issues with unflattened headers, was just attempting to reuse function that was already available to ensure the lowercasing. Happy to take a another pass to limit the operations to just casing in the interest of perf.

I changed to not lowercase because the headers are always lowercase when came from API Gateway V1, so for performance reasons, I just skipped the lowercase.

Very interesting! This has not been our observation. After updating from v2.17.0 to v4.2.1, we noticed our services were not receiving cookies anymore which ended up being due to the header now being the capitalized Cookie instead of just cookie. Downgrading serverless adapter resolved the issue.

@H4ad
Copy link
Owner

H4ad commented Sep 5, 2024

Downgrading serverless adapter resolved the issue.

You can just copy the adapter that you modified to your own codebase and keep the newer version of the library.

which ended up being due to the header now being the capitalized Cookie instead of just cookie.

I'm fine with having back the behavior but this will require a breaking change release.

If you want to merge this PR and release as a feat, you can modify this PR and create a new flag option for this adapter called shouldLowercaseHeaders and make it default to false. In this way, you can mark it as true for your use-case and we can see if more people expect the header to be lowercase, what do you think?

@tcandens
Copy link
Author

tcandens commented Sep 6, 2024

Thanks @H4ad,
I've reworked the PR to involve an option to pass to the adapter constructor to avoid a breaking change.

Please let me know what you think.

H4ad
H4ad reviewed Sep 6, 2024
View reviewed changes
Copy link
Owner

@H4ad H4ad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two little changes/question but looks good overall!

src/core/headers.ts Outdated Show resolved Hide resolved
src/adapters/aws/api-gateway-v1.adapter.ts Outdated Show resolved Hide resolved
fix(apig-v1-adapter): lowercase request headers
4fbb588 
remove log

rework lowercase request header options

performance minded util function

remove unneeded option assignment

add some docs

grammar

tweaks

prefer for..of loop
@tcandens
Copy link
Author

tcandens commented Sep 6, 2024

Thanks again @H4ad,
Made two small tweaks. Please review

@H4ad
Copy link
Owner

H4ad commented Sep 6, 2024

LGTM, thanks for the contribution!

@H4ad H4ad merged commit 0e161a1 into H4ad:main Sep 6, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@H4ad H4ad H4ad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tcandens @H4ad